-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Continue counting other perf events group, even if there is an error in one #8
base: master
Are you sure you want to change the base?
Conversation
Update fork
Is it possible to add tests to cover changes which you've made in the code? |
c6b3e9a
to
987a8b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @Creatone please make also a review
987a8b1
to
c699255
Compare
perf/collector_libpfm_test.go
Outdated
@@ -197,6 +199,34 @@ func TestNewCollector(t *testing.T) { | |||
assert.Same(t, &perfCollector.events.Core.CustomEvents[0], perfCollector.eventToCustomEvent[Event("event_2")]) | |||
} | |||
|
|||
func TestCollectorSetup(t *testing.T) { | |||
path, err := mockSystemDevices() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mockSystemDevices
create a directory similar to /sys/devices/
with some uncore PMUs.
Perf collector needs cgroup path to pass it's identity to perfEventOpen
and ioctlSetInt
.
Why does this work? Because in this scenario you need only a directory with reading rights.
Try using ioutil.TempDir("", "cgroup")
perf/collector_libpfm_test.go
Outdated
@@ -20,6 +20,8 @@ package perf | |||
import ( | |||
"bytes" | |||
"encoding/binary" | |||
"golang.org/x/sys/unix" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate this import.
Core: Events{ | ||
Events: []Group{ | ||
{[]Event{"cache-misses"}, false}, | ||
{[]Event{"non-existing-event"}, false}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one! 👍
leaderFileDescriptors, err := c.createLeaderFileDescriptors(group.events, cgroupFd, groupIndex, leaderFileDescriptors) | ||
if err != nil { | ||
klog.Error(err) | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot about cleaning after the group. This leads to errors when reading stats.
Consider using this function
func (c *collector) deleteGroup(index int) {
for name, files := range c.cpuFiles[index].cpuFiles {
for cpu, file := range files {
klog.V(5).Infof("Closing perf_event file descriptor for cgroup %q, event %q and CPU %d", c.cgroupPath, name, cpu)
err := file.Close()
if err != nil {
klog.Warningf("Unable to close perf_event file descriptor for cgroup %q, event %q and CPU %d", c.cgroupPath, name, cpu)
}
}
}
delete(c.cpuFiles, index)
}
This is part of code from Destroy()
. So it can look now like:
func (c *collector) Destroy() {
c.uncore.Destroy()
c.cpuFilesLock.Lock()
defer c.cpuFilesLock.Unlock()
for i, _ := range c.cpuFiles {
c.deleteGroup(i)
}
}
perf/uncore_libpfm.go
Outdated
@@ -202,7 +202,7 @@ func (c *uncoreCollector) setup(events PerfEvents, devicesPath string) error { | |||
} | |||
|
|||
if err != nil { | |||
return err | |||
klog.Error(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, count the entire group or skip it. So in this scenario, you need also to delete the group like in collector
.
perf/collector_libpfm.go
Outdated
} else { | ||
config, err := c.createConfigFromEvent(event) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable create config from perf event %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got: unable create config from perf event unable to transform event name event-that-not-exists to perf_event_attr: -4
Can it look better?
perf/collector_libpfm.go
Outdated
config := c.createConfigFromRawEvent(customEvent) | ||
leaderFileDescriptors, err = c.registerEvent(eventInfo{string(customEvent.Name), config, cgroupFd, groupIndex, isGroupLeader}, leaderFileDescriptors) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to register perf event %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to config error.
perf/collector_libpfm.go
Outdated
} | ||
leaderFileDescriptors, err = c.registerEvent(eventInfo{string(event), config, cgroupFd, groupIndex, isGroupLeader}, leaderFileDescriptors) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to register perf event %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to config error.
c699255
to
104d8aa
Compare
perf/uncore_libpfm.go
Outdated
@@ -202,9 +202,15 @@ func (c *uncoreCollector) setup(events PerfEvents, devicesPath string) error { | |||
} | |||
|
|||
if err != nil { | |||
return err | |||
klog.Error(err) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this blank line.
Maybe we can unify logs in this PR? What do you think? |
32dcffb
to
60a7f41
Compare
…in one Signed-off-by: Wisniewski, Krzysztof2 <[email protected]>
60a7f41
to
5f78d87
Compare
Signed-off-by: Wisniewski, Krzysztof2 <[email protected]>
73d7734
to
cf09a32
Compare
59f8a19
to
7179a92
Compare
Signed-off-by: Wisniewski, Krzysztof2 <[email protected]>
7179a92
to
a1dee01
Compare
Signed-off-by: Wisniewski, Krzysztof2 [email protected]